-
Notifications
You must be signed in to change notification settings - Fork 932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove outdated "single sourcing package version" advice #1276
Conversation
I mostly agree. But completely deleting the page seems too radical. I guess it would make sense to still provide some guidance on this topic. At the very least we should tell to use |
Please no. The rejection of PEP 396 means that the SC doesn't want to say you should use a Having a version attribute is widely done, a good idea (IMHO) and supported by the up-to-date packaging tools (setuptools, etc). I hope we all agree that if you are going to use a version I also note that at least originally, the conclusion of the thread in #182 was that PyPA would not endorse one single way do things -- adding a |
@ChrisBarker-NOAA Could you explain why you think having a version attribute is a good idea? You did say "I really like Here are some arguments against burning a version string into the source:
So, there are a bunch of disadvantages. What are the advantages? Do they outweigh the disadvantages? |
There is also the fact that an import package is not the same thing as a distribution package. It is a subtle detail and all things considered it might not matter much for this task, but there is still a difference. When I do |
I thought about writing all that, but this really isn't the place for that discussion. I don't have any particular pull here, but I think the goal of these docs is to document the state of practice -- and having a
Yes, indeed -- which if one reason I prefer version attribute :-)
This entire page is about removing that redundancy, isn't it?
Only if the importable package name is the same as the distribution name.
yeah, which is why I thought PEP 396 was a good idea -- but even since that PEP was written, there's been a LOT more standardization in the community -- but another topic anyway -- at least now there IS a standard for what a version string should look like, THAT doesn't need to change. But again, not the place for this debate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm strongly against blindly removing the guide. Its point is not to provide an importable, such as __version__
in an installed package but quite vice versa — show ways for the version value to make its way into the installed package metadata. And an importable variable is just one of the sources, many people use SCM plugins that can pull that from Git tags and so on.
So depending on where the source of truth is, different projects would have different assumptions around handling/generation/presence of __version__
(some use different variable names but the purpose is the same).
@webknjaz This is not blindly removing the guide. It is removing the guide with a reasoned set of arguments, as described in the comments here. Regardless of where the source of truth is, the premise that a What changes are you requesting? |
We can rename it to But as I said before, single-sourcing of a version value is a legit use case and is what people often aspire to. Regardless of what the source is or how it's called. |
This dismisses the case when a library/app itself makes use of the version and it does need a variable to store it somewhere. And this is one of the aspects of single-sourcing — the version is a part of the metadata but it's often also necessary in runtime. And yes, that variable can be inferred from the installed package metadata, if it's a library. But many people have other preferences. Also, apps are often not packaged as dists and so they need to rely on different sources of versioning or the mechanism of generating them. I think it's acceptable to augment the guide with clarifications and other example but dismissing it as a whole and pretending that people don't do this is rather harmful. |
Exactly. Moreover, some PEPs, even being rejected, end up being widely used across the ecosystem, as One other case of a "rejected" concept is "attribute docstrings" — https://peps.python.org/pep-0258/#attribute-docstrings. The accepted PEP 257 shows some legit use of of "rejected attribute docstrings": https://peps.python.org/pep-0257/#what-is-a-docstring. Back to By the way, there's a PEP 723 draft in the works: https://peps.python.org/pep-0723/. This essentially allows moving |
This is not always possible since the dist name can contain dashes that aren't allowed in Python identifiers. Also, a standalone script might be functional even without being packaged or |
If we want to keep this page of the guide, then there is a whole bunch of clarifications to be made.
|
Yet, the version can be sourced dynamically on build, which makes external resources like Git the actual source of truth and the metadata is an intermediate snapshot of that data from SCM (prone to becoming outdated in cases like development w/ editable installs). Another case is when a project uses said variable as its ultimate source of truth. With this in mind, that variable would be external to the build system generating the version, just like Git/SCM might be. With a version in I think there's some confusion as to how versions are being used and what single-sourcing is about. In terms of packaging, that version sourcing is about getting the version number from somewhere and putting it into the metadata. In terms of runtime, it's about getting the version from somewhere (like metadata) and making use of it in a context that is not related to packaging. So if we present this as a unidirectional flow, it'd be something like this: ---
title: Version sourcing flow
---
graph LR;
single-source[("
Non-standardized ultimate version source:
- build system config file
- CLI flag/option/argument
- environment variable
- *.py module import
- static parsing of a *.py module as a text file
- Git/setuptools-scm
- API call
- database lookup
")];
meta[(True-to-spec package metadata)];
runtime{{"
occasional access to the metadata version,
maybe through __version__
"}};
single-source --> meta --> runtime;
runtime -. runtime may sometimes be the source but it is mostly usable for pure-python projects .-> single-source;
The guide is describing the above, or at least, a part of it. And I think it should remain, possibly be improved with additions, but not deleted. |
@sinoroc yep, I agree with all the points. |
We should probably ask opinions of @RonnyPfannschmidt and @jaraco, though, as they have a lot of experience with this specific topic. |
Oh, and I forgot to mention that dists may ship multiple importable packages/modules, not just one. So on distribution name can be mapped to multiple importables. Such dists are |
@flying-sheep @abravalheri I saw you commenting on the neighboring PR so FYI ^ |
i like the motion of going away from that being said, there probably ought to be a section on providing a |
I'm going to rephrase @webknjaz's comment: I'm strongly against removing the guide with eyes wide open. There is NOT a consensus yet about the "one true way" to handle version specification, but there IS consensus that at the very least, however you do the version specification should be DRY -- i.e. "One source of truth" -- and THAT is what this page is about. So it still serves a purpose, and I don't think it should be simply removed unless / until a proper consensus is reached. And as @webknjaz said this guide does not say "you shold put a Anyway, I think there is also a consensus that the text as it stands is pretty darn out of date, so keeping the page as is is a disservice to the community. NOTE: Could we please merge a PR along the lines of: #1273 [*] sooner than later, while the discussion continues about the ultimate one true way? Where should that discussion be held? I'm not sure this PR is the right place. [*] "along the lines of" could be a slightly edited version (I'll go now and address a few comments) - or a bigger re-write if someone wants to tackle it. Maybe put the "extract version from SCM" at the top :-) -- note that when I wrote that PR, I purposely made as few changes as possible, I naively thought that would make it less controversial. So maybe a larger change would be better -- perhaps completely remove the "no longer recommended" section? |
a963e99
to
6fef34c
Compare
IMO there is nothing valuable worth saving from the existing page, but I wouldn't be against a total re-write that is build backend agnostic. I'm not sure what's the urgency for merging #1273 - it still has several problems, and it's still setuptools/distutils specific. |
@ChrisBarker-NOAA thanks for summarizing the above! I think your PR has potential! @wimglenn the most important bit of this guide is "how to bring the version string from somewhere, into the package metadata". Removing it just because of a side effect of how the version is additionally accessed in runtime is not an option. You've been making arguments about the runtime (right side of my illustration) but it does hurt the usefulness of the left-side part — the build process. |
My view is that:
Personally, I would not completely remove all the guidance for single-sourcing, even if it is backend specific, because that is very useful for the users. If the website mainters prefer to not have backend-specific instructions, we can transfer the setuptools-specific docs to the setuptools website, but it would be good if we can leave at least a link so users can be pointed out in the right direction. |
Hi @wimglenn. Thanks for the PR. I think the consensus is leaning toward updating the doc instead of removing it. I'm linking to my review #1273 (comment) on #1273. |
I'd also prefer Chris's change to go through (and I'm coming from the |
It looks like other efforts are no longer attempting to modify that page, but instead add a new page / rewrite. So, this can be merged in the meantime. These PRs are not blocking each other any longer. @webknjaz Can you remove your change request? |
Now that #1580 has been merged, I think we can merge this -- or at least delete the out-of-date page in "guides" -- which is most of this PR. |
This PR intends to remove the "single sourcing package version" advice from the guides section. Since there is now a version of the page under Also, since there was concern about having broken links, is it possible to just create a URL redirect from |
URL redirect makes more sense than a single-line page, but it has to be set up in readthedocs AFAIK and can’t be done in this PR. |
sound great -- can someone add it to this PR? |
…ting-pyproject.toml
Add link to discussion "Single-sourcing the Project Version" in writing-pyproject.toml
done. Thank you Wim for merging. |
Coming here from https://discuss.python.org/t/please-make-package-version-go-away/58501/53 where I've suggested:
|
@pradyunsg: At this point, this PR is only cleaning up some cross referencing -- not anything substantial -- so I think it's good to merge. |
Checking in again, @webknjaz is there anything else that you would want seen changed to this PR before feeling comfortable merging? |
@tabedzki honestly, I was under the impression that I relayed my opinion that this content needs to be updated instead of removed. Plus, I lurked into the dpo thread that Pradyun linked, and I kinda agree that this one shouldn't be rushed. And Paul made a point that perhaps I merged the other PR too soon. This makes me think that I made some assumptions too without clearly communicating them. But the bottom line is that I'd like to see a recipe of how to do single-source versioning remaining in PyPUG. It can be updated to show more modern techniques, but I don't want it to vanish — I think this is wrong. It would probably be a good idea to rename one of the pages so they are titled differently. |
I think your concerns are all addressed. The title of this is a bit out of date-- there have been other PRs merged that updated and moved the discussion of versioning (due to the restructuring of the docs) -- I think at this point, this one simply cleans up the kruft and cross referencing. There is now a discussion under "discussions": https://packaging.python.org/en/latest/discussions/single-source-version/ So the topic hasn't been been abandoned. Whether that should be under "discussions" or "Guides" is another question -- though looking again, I think Discussions is correct) I think this PR is simply:
So I think we're good to go with a merge NOTE: I was the one that started all this and very much wanted it to remain documented :-( |
@webknjaz It's not being rushed, it has been open for over a year and the branch has been rebased or merged with main at least 6 times. Many people have reviewed and commented here, and it looks like a majority agree on removing this bad/outdated page. 100% consensus shouldn't be necessary, progress would be impossible if that were a requirement in PyPA. I think using the request changes GitHub feature to "sit" on a PR is not proper. Use of this feature should clearly communicate what changes are being requested. From earlier comments it sounds like you're asking for a rewrite/replacement of the page in this PR, which is not something I'm going to do. Person(s) who believe there's value providing a version attribute should write it, add some justification of why to have an attribute, and demonstrate how to do it well. My fork/branch is not the place to write that and I'm not a good person to write it, because I don't advocate for this practice in the first place. The PR is about removing the existing page. I think it should be removed immediately. Removing it does not block any attempt to write a replacement, on the contrary it gives someone a clean slate to do so. The guide is not code, there's no risk of breaking users by removing an outdated page in the meantime. If you think it shouldn't be removed, and you feel strongly enough about that to override the approvals of Jason and Ronny, then please just close the PR (I won't be upset!). If you're happy for the existing page to be gone if and when someone is working on a replacement, then why not merge the PR. |
The replacement has ALREADY BEEN WRITTEN. I've mentioned that twice. Here it is again: https://packaging.python.org/en/latest/discussions/single-source-version/ Really I was the one that started all this, and was (and am) a strong advocate of keeping a discussion of this topic in the docs -- I wrote that other page -- there is no reason not to merge this PR. (I wish I'd thought to remove it in the PR that I added that other page with ... oh well.) Granted that page is pretty slim, but that's because: a) It's still controversial, so I had to stick with just the facts without seeming to endorse a particular approach. b) The details of how to do it depend on the build system used -- back in the day, there was just setuptools, no longer. So be refer folks to docs for their build system. If you think that page should be fleshed out more, then submit a PR about that -- but keeping this really old totally out of date page around is not helpful. |
I agree, either we merge this PR or close it. This is about removing the outdated guide. There are not 2 million ways of removing a page. If there are suggestions on how to better remove this page, then let's put them forward and get moving. If one wants to discuss how to rewrite this guide, then let's do it somewhere else. |
#1607 is a PR that updates the new discussion, and replaces the old page with a HTTP redirect to the discussion page. |
I've added @wimglenn as a co-author on the updated PR (since it does include some of their pending changes). Thanks for continuing to push for this to be cleaned up! |
Now that Python 3.7 is EOL and stdlib
importlib.metadata
is available in all non-EOL Python versions, it is no longer advisable to keep a__version__
attribute in the source code. Useimportlib.metadata.version()
to get it from package metadata if necessary.References:
.__version__
attributes are IMNSHO generally pointless" - gpshead__version__
is a fairly ingrained habit for a lot of people. But it’s a holdover from the days beforeimportlib.metadata
." - pf_moore__version__
attribute at all?" - me